Implement OFAC list address check #346
Conversation
WalkthroughThe changes introduce a compliance mechanism that checks Ethereum addresses against the Office of Foreign Assets Control (OFAC) sanctions list. A new JSON file containing sanctioned addresses is added, along with an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Composer
participant OfacChecker
participant Broadcaster
User->>Composer: Request MsgGrantGeneric
Composer->>OfacChecker: Check if granter is blacklisted
OfacChecker-->>Composer: Return blacklist status
alt Not Blacklisted
Composer-->>User: Grant authorization
else Blacklisted
Composer-->>User: Raise exception
end
User->>Broadcaster: Request to broadcast message
Broadcaster->>OfacChecker: Validate trading address
OfacChecker-->>Broadcaster: Return blacklist status
alt Not Blacklisted
Broadcaster-->>User: Broadcast message
else Blacklisted
Broadcaster-->>User: Raise exception
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
3 times, most recently
from
8069fb6
to
4ff81ed
Compare
There was a problem hiding this comment.
Hi @shibaeff. It is great to see your first PR.
Please, change the base branch. In general for all PR (unless it is for a fix we need to create a version for really quick) all new PRs should be created from the dev branch
I have added some comments on the code to start conversations.
Also, according to the task definition provided by Bojan, we need to add a validation in the authz grant messages in the composer.py module:
- MsgGrantTyped
- MsgGrantGeneric
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
4 times, most recently
from
8f3e91e
to
33b4de3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (4)
pyinjective/core/ofac.py (1)
21-25: Consider using a more flexible approach for determining the OFAC list path.The
get_ofac_list_pathmethod is assuming a specific directory structure relative to the current working directory. This may not work in all environments or if the script is run from a different directory. Consider using a more flexible approach, such as:
- Allowing the path to be configured via an environment variable or a configuration file.
- Using a fixed path relative to the current file's directory.
Here's an example of using a fixed path relative to the current file's directory:
@classmethod def get_ofac_list_path(cls): current_directory = os.path.dirname(os.path.abspath(__file__)) return os.path.join(current_directory, OFAC_LIST_FILENAME)This assumes that the OFAC list file is located in the same directory as the
ofac.pyfile.pyinjective/core/broadcaster.py (1)
66-69: LGTM, but consider adding a unit test.The changes to check if the trading address is blacklisted and raise an exception look good. This is a critical compliance check.
Consider adding a unit test that verifies an exception is raised when initializing
BroadcasterAccountConfigwith a blacklisted trading address. This will help ensure the compliance check works as expected.pyinjective/composer.py (2)
476-478: LGTM, but consider adding a unit test.The changes to check if the granter address is blacklisted and raise an exception in
MsgGrantGenericlook good. This is a critical compliance check.Consider adding a unit test that verifies an exception is raised when calling
MsgGrantGenericwith a blacklisted granter address. This will help ensure the compliance check works as expected.
2132-2134: LGTM, but consider adding a unit test.The changes to check if the granter address is blacklisted and raise an exception in
MsgGrantTypedlook good. This is a critical compliance check.Consider adding a unit test that verifies an exception is raised when calling
MsgGrantTypedwith a blacklisted granter address. This will help ensure the compliance check works as expected.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- ofac.json (1 hunks)
- pyinjective/composer.py (4 hunks)
- pyinjective/core/broadcaster.py (2 hunks)
- pyinjective/core/ofac.py (1 hunks)
- tests/core/test_broadcaster.py (1 hunks)
Additional context used
Ruff
tests/core/test_broadcaster.py
32-32:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/core/ofac.py
41-41: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (4)
ofac.json (1)
1-1: LGTM!The JSON file containing the OFAC sanctioned addresses looks good. It's a simple array of Ethereum addresses.
pyinjective/core/broadcaster.py (1)
13-13: LGTM!The import of the
OfacCheckerclass looks good.pyinjective/composer.py (2)
13-13: LGTM!The import of the
OfacCheckerclass looks good.
151-151: LGTM!Initializing the
OfacCheckerinstance in the constructor looks good.
tests/core/test_broadcaster.py
Outdated
| composer = Mock(spec=Composer) | ||
|
|
||
| account_config = StandardAccountBroadcasterConfig(private_key=private_key_banned.to_hex()) | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
Use a more specific exception in the pytest.raises assertion.
The test is using a generic Exception in the pytest.raises assertion. It's better to use a more specific exception that is actually raised by the MsgBroadcasterWithPk constructor when the address is banned.
Modify the assertion to use the specific exception:
with pytest.raises(SpecificException):
...Replace SpecificException with the actual exception raised by the constructor.
Tools
Ruff
32-32:
pytest.raises(Exception)should be considered evil(B017)
tests/core/test_broadcaster.py
Outdated
| address_banned = public_key_banned.to_address() | ||
|
|
||
| ofac_list = [address_banned.to_acc_bech32()] | ||
| ofac.OFAC_LIST_FILENAME = "ofac_test.json" |
There was a problem hiding this comment.
Use a unique filename for the mock OFAC list in this test.
The test is modifying the global OFAC_LIST_FILENAME variable, which may affect other tests. It's better to use a unique filename for the mock OFAC list in this test.
Modify the code to use a unique filename:
ofac.OFAC_LIST_FILENAME = "ofac_test_broadcaster.json"Make sure to update the filename in the os.remove call at the end of the test as well.
tests/core/test_broadcaster.py
Outdated
| with open(ofac.OfacChecker.get_ofac_list_path(), "w") as ofac_file: | ||
| json.dump(ofac_list, ofac_file) | ||
|
|
||
| network = Network.local() | ||
| client = AsyncClient( | ||
| network=Network.local(), | ||
| ) | ||
| composer = Mock(spec=Composer) | ||
|
|
||
| account_config = StandardAccountBroadcasterConfig(private_key=private_key_banned.to_hex()) | ||
| with pytest.raises(Exception): | ||
| _ = MsgBroadcasterWithPk( | ||
| network=network, | ||
| account_config=account_config, | ||
| client=client, | ||
| composer=composer, | ||
| fee_calculator=Mock(), | ||
| ) | ||
| os.remove(ofac.OfacChecker.get_ofac_list_path()) |
There was a problem hiding this comment.
Use a try...finally block to ensure cleanup of the mock OFAC list file.
The test is creating a mock OFAC list file but is not cleaning it up in case of an exception. It's better to use a try...finally block to ensure the file is always deleted after the test.
Modify the code to use a try...finally block:
try:
with open(ofac.OfacChecker.get_ofac_list_path(), "w") as ofac_file:
json.dump(ofac_list, ofac_file)
# ... rest of the test code ...
finally:
os.remove(ofac.OfacChecker.get_ofac_list_path())
ofac.OFAC_LIST_FILENAME = "ofac.json"Tools
Ruff
32-32:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/core/ofac.py
Outdated
| except (aiohttp.ClientError, json.JSONDecodeError) as e: | ||
| raise Exception(f"Error fetching OFAC list: {e}") |
There was a problem hiding this comment.
Raise a more specific exception or let the original exception propagate.
The download_ofac_list method is raising a generic Exception with a custom error message. It's better to raise a more specific exception, such as IOError or ValueError, depending on the type of error. Alternatively, you can let the original exception propagate by using raise without arguments.
Modify the code to raise a more specific exception or let the original exception propagate:
except (aiohttp.ClientError, json.JSONDecodeError) as e:
raise IOError(f"Error fetching OFAC list: {e}") from eOr:
except (aiohttp.ClientError, json.JSONDecodeError) as e:
raiseTools
Ruff
41-41: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
from
33b4de3
to
04a6f94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- ofac.json (1 hunks)
- pyinjective/composer.py (4 hunks)
- pyinjective/core/broadcaster.py (2 hunks)
- pyinjective/core/ofac.py (1 hunks)
- pyproject.toml (1 hunks)
- tests/core/test_broadcaster.py (1 hunks)
Files skipped from review due to trivial changes (2)
- ofac.json
- pyproject.toml
Files skipped from review as they are similar to previous changes (2)
- pyinjective/composer.py
- pyinjective/core/broadcaster.py
Additional context used
Ruff
tests/core/test_broadcaster.py
32-32:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/core/ofac.py
41-41: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (4)
tests/core/test_broadcaster.py (3)
21-21: The previous review comment suggesting the use of a unique filename for the mock OFAC list in this test is still valid and applicable to the current code segment.
22-40: The previous review comment suggesting the use of atry...finallyblock to ensure cleanup of the mock OFAC list file is still valid and applicable to the current code segment.Tools
Ruff
32-32:
pytest.raises(Exception)should be considered evil(B017)
32-32: The previous review comment suggesting the use of a more specific exception in thepytest.raisesassertion is still valid and applicable to the current code segment. The static analysis tool Ruff also flags this as an issue.Tools
Ruff
32-32:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/core/ofac.py (1)
40-41: The previous review comment suggesting raising a more specific exception or letting the original exception propagate is still valid and applicable to the current code segment. The static analysis tool Ruff also flags this as an issue.Tools
Ruff
41-41: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
2 times, most recently
from
bf70a17
to
71d9b6a
Compare
|
@PavelInjective I am reviewing now the PR. In the meantime, could you please move the |
pyinjective/ofac.py
Outdated
| def __init__(self): | ||
| self._ofac_list_path = self.get_ofac_list_path() | ||
| if not os.path.exists(self._ofac_list_path): | ||
| self.download_ofac_list() |
There was a problem hiding this comment.
Since now the download_afac_list() method is async, it can't be called like this (as if it were a sync method). Also, we won't be able to call an async method from init that is sync.
I think we will need to make the list lazy initialized to properly use the async download_ofac_list.
Other alternative is to always use only the local file, and leave the async download method only to be used when updating the list manually.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- ofac.json (1 hunks)
- pyinjective/composer.py (4 hunks)
- pyinjective/core/broadcaster.py (2 hunks)
- pyinjective/ofac.py (1 hunks)
- tests/core/test_broadcaster.py (1 hunks)
Files skipped from review due to trivial changes (1)
- ofac.json
Files skipped from review as they are similar to previous changes (2)
- pyinjective/composer.py
- pyinjective/core/broadcaster.py
Additional context used
Ruff
tests/core/test_broadcaster.py
32-32:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/ofac.py
41-41: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (7)
tests/core/test_broadcaster.py (4)
41-41: The existing comment suggesting moving the code that modifies theOFAC_LIST_FILENAMEto thesetUpandtearDownfunctions is still valid. Please implement the suggested changes.
22-40: The existing comment suggesting using atry...finallyblock to ensure cleanup of the mock OFAC list file is still valid. Please implement the suggested changes.Tools
Ruff
32-32:
pytest.raises(Exception)should be considered evil(B017)
21-21: The existing comment suggesting using a unique filename for the mock OFAC list in this test is still valid. Please implement the suggested changes.
32-32: The existing comment suggesting using a more specific exception in thepytest.raisesassertion is still valid. Please implement the suggested changes.Tools
Ruff
32-32:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/ofac.py (3)
12-15: Fix the issue with calling the asyncdownload_ofac_listmethod from the sync__init__method.The
download_ofac_listmethod is async but it's called from the sync__init__method. This will not work as expected.As suggested in the previous review, make the list lazy initialized to properly use the async
download_ofac_listmethod. Alternatively, always use only the local file, and leave the async download method only to be used when updating the list manually.
36-38: Usejson.dumpwith indentation to have the list printed one address per line.As suggested in the previous review, use
json.dumpwith indentation to have the list printed one address per line:json.dump(ofac_list, f, indent=2)
47-49: LGTM!The code changes are approved.
pyinjective/ofac.py
Outdated
| except (aiohttp.ClientError, json.JSONDecodeError) as e: | ||
| raise Exception(f"Error fetching OFAC list: {e}") |
There was a problem hiding this comment.
Distinguish the raised exception from errors in exception handling.
As suggested by the static analysis tool, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling:
| except (aiohttp.ClientError, json.JSONDecodeError) as e: | |
| raise Exception(f"Error fetching OFAC list: {e}") | |
| except (aiohttp.ClientError, json.JSONDecodeError) as e: | |
| raise Exception(f"Error fetching OFAC list: {e}") from e |
Tools
Ruff
41-41: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
There was a problem hiding this comment.
Why are we handling only this two exceptions?
There was a problem hiding this comment.
@aarmoa, you're right to point out that only two exceptions are being handled here. The current implementation focuses on two specific failure points:
aiohttp.ClientErrorfor network-related issues.json.JSONDecodeErrorfor JSON parsing issues.
However, there are indeed other potential exceptions that could occur, such as file I/O errors when writing the file or other unforeseen issues. It would make sense to expand the exception handling to cover these cases as well. I'll suggest adding broader exception handling for file operations and any other unexpected errors to ensure robustness.
Let me know if you'd like me to propose specific changes!
There was a problem hiding this comment.
now we handle the general Exception
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
from
5dec907
to
0131754
Compare
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
from
0131754
to
a0c40d2
Compare
pyinjective/ofac.py
Outdated
| except (aiohttp.ClientError, json.JSONDecodeError) as e: | ||
| raise Exception(f"Error fetching OFAC list: {e}") |
There was a problem hiding this comment.
Why are we handling only this two exceptions?
PavelInjective
force-pushed
the
ofac-python3-sdk
branch
from
a0c40d2
to
c1487d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- pyinjective/composer.py (4 hunks)
- pyinjective/core/broadcaster.py (2 hunks)
- pyinjective/ofac.json (1 hunks)
- pyinjective/ofac.py (1 hunks)
- tests/core/test_broadcaster.py (1 hunks)
Files skipped from review due to trivial changes (1)
- pyinjective/ofac.json
Files skipped from review as they are similar to previous changes (2)
- pyinjective/composer.py
- pyinjective/core/broadcaster.py
Additional context used
Ruff
tests/core/test_broadcaster.py
41-41:
pytest.raises(Exception)should be considered evil(B017)
pyinjective/ofac.py
44-44: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Additional comments not posted (5)
tests/core/test_broadcaster.py (2)
17-26: The previous review comments regarding thesetUpmethod are still valid and applicable. No further comments are necessary.
28-30: The previous review comments regarding thetearDownmethod are still valid and applicable. No further comments are necessary.pyinjective/ofac.py (3)
50-51: LGTM!The code changes are approved.
54-55: LGTM!The code changes are approved.
13-21: Consider lazy-loading the OFAC list.As mentioned in the previous review comment, the
__init__method could be improved by lazy-loading the OFAC list instead of loading it in the constructor. This would allow thedownload_ofac_listmethod to be called asynchronously to update the list.Here's a suggestion for lazy-loading the OFAC list:
def __init__(self): self._ofac_list_path = self.get_ofac_list_path() self._ofac_list = None def _load_ofac_list(self): if not os.path.exists(self._ofac_list_path): raise Exception( "OFAC list is missing on the disk. Please, download it by running python3 pyinjective/ofac_list.py" ) with open(self._ofac_list_path, "r") as f: self._ofac_list = set(json.load(f)) def is_blacklisted(self, address: str) -> bool: if self._ofac_list is None: self._load_ofac_list() return address in self._ofac_list
|
|
||
| account_config = StandardAccountBroadcasterConfig(private_key=self.private_key_banned.to_hex()) | ||
|
|
||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
Use a more specific exception in the pytest.raises assertion.
The test is using a generic Exception in the pytest.raises assertion. It's better to use a more specific exception that is actually raised by the MsgBroadcasterWithPk constructor when the address is banned.
Modify the assertion to use the specific exception:
with pytest.raises(SpecificException):
...Replace SpecificException with the actual exception raised by the constructor.
Do you want me to update the test with the specific exception or open a GitHub issue to track this improvement?
Tools
Ruff
41-41:
pytest.raises(Exception)should be considered evil(B017)
| async def download_ofac_list(cls): | ||
| async with aiohttp.ClientSession() as session: | ||
| try: | ||
| async with session.get(OFAC_LIST_URL) as response: | ||
| response.raise_for_status() | ||
| text_content = await response.text() | ||
| ofac_list = json.loads(text_content) | ||
| ofac_file_path = cls.get_ofac_list_path() | ||
| with open(ofac_file_path, "w") as f: | ||
| json.dump(ofac_list, f, indent=2) | ||
| f.write("\n") | ||
| return | ||
| except Exception as e: | ||
| raise Exception(f"Error fetching OFAC list: {e}") |
There was a problem hiding this comment.
Handle more exceptions and use raise ... from to distinguish the raised exception.
The download_ofac_list method could be improved by handling more exceptions, such as file I/O errors when writing the file or other unforeseen issues. Additionally, as suggested by the static analysis tool, use raise ... from to distinguish the raised exception from errors in exception handling.
Here's a suggestion for handling more exceptions and using raise ... from:
@classmethod
async def download_ofac_list(cls):
async with aiohttp.ClientSession() as session:
try:
async with session.get(OFAC_LIST_URL) as response:
response.raise_for_status()
text_content = await response.text()
ofac_list = json.loads(text_content)
ofac_file_path = cls.get_ofac_list_path()
try:
with open(ofac_file_path, "w") as f:
json.dump(ofac_list, f, indent=2)
f.write("\n")
except IOError as e:
raise Exception(f"Error writing OFAC list to file: {e}") from e
return
except (aiohttp.ClientError, json.JSONDecodeError) as e:
raise Exception(f"Error fetching OFAC list: {e}") from e
except Exception as e:
raise Exception(f"Unexpected error: {e}") from eTools
Ruff
44-44: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Summary by CodeRabbit
New Features
Bug Fixes
Tests